Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[python-package] allow use of early_stopping_round<=0 to turn off early stopping (fixes #6401) #6406

Merged
merged 10 commits into from
Apr 20, 2024
Merged

[python-package] allow use of early_stopping_round<=0 to turn off early stopping (fixes #6401) #6406

merged 10 commits into from
Apr 20, 2024

Conversation

ddelzell
Copy link
Contributor

@ddelzell ddelzell commented Apr 3, 2024 •
edited by jameslamb
Loading

Fixes #6401

The early stopping callback was instantiated if early_stopping_round was found in the parameter list, even its values was <=0 (which should disable early stopping). The code was modified to only instantiate the early stopping callback if early_stopping_round was both an integer and >0.

Various error messages were updated and unit tests were added.

@ddelzell
Copy link
Contributor Author

ddelzell commented Apr 3, 2024

@microsoft-github-policy-service agree company="Grubhub"

@jameslamb jameslamb added the fix label Apr 3, 2024
@jameslamb
Copy link
Collaborator

Thanks! I've added the statement "fixes #6401" to the description. In case you're not familiar with GitHub... that's helpful for 2 reasons:

  1. it means [python-package] Early stopping callback added when early_stopping_round = 0 #6401 will automatically be closed when we merge this PR
  2. it means that anyone finding [python-package] Early stopping callback added when early_stopping_round = 0 #6401 from search will see a link back to this PR
Screenshot 2024-04-03 at 2 32 57 PM

More details on that: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

@ddelzell
Copy link
Contributor Author

ddelzell commented Apr 3, 2024

Ah, looks like I forgot to run some linting pre-commit hooks? I can try to fix those. Also, we typically rebase to one commit in our PRs as we make changes. Do you have any problem with that?

jameslamb
jameslamb requested changes Apr 3, 2024
View reviewed changes
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for doing this! I left one comment for you to consider. Will try to provide a more thorough review later.

python-package/lightgbm/engine.py Outdated Show resolved Hide resolved
@jameslamb
Copy link
Collaborator

Ah, looks like I forgot to run some linting pre-commit hooks? I can try to fix those

Yep. You can do this from the root of the repo to run them.

pip install pre-commit
pre-commit run --all-files

we typically rebase to one commit in our PRs as we make changes. Do you have any problem with that?

We squash all commits into 1 when merging here, so we end up with 1 commit per PR. Check this out: https://github.com/microsoft/LightGBM/commits/master/

Because of that:

  • you could push 1 or 10 or 100 commits to this PR and it won't be a problem
  • you don't need to worry about commit messages being useful... only the PR title will be used as a commit message when this is merged

So please don't rebase and force-push here. It's useful for reviewers to be able to see the individual changes being made in response to review comments, and all the CI logs are tied to commits so it's useful to be able to compare consecutive CI runs.

@jameslamb jameslamb changed the title Remove early stopping callback addition when stopping_rounds <=0 [python-package] allow use of early_stopping_round<=0 to turn off early stopping Apr 3, 2024
@ddelzell ddelzell requested a review from jameslamb April 4, 2024 17:24
jameslamb
jameslamb requested changes Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for this! The tests look great, I just left some small suggestions. This is pretty close, and we'll get it into the next release.

python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
python-package/lightgbm/callback.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_callback.py Show resolved Hide resolved
@ddelzell ddelzell requested a review from jameslamb April 5, 2024 17:46
@jameslamb jameslamb marked this pull request as ready for review April 10, 2024 13:37
@jameslamb
Copy link
Collaborator

Thanks for the changes!

I've updated this to the latest state of the master branch and converted it from "draft" to "ready for review", to pull in some fixes to CI that were preventing a lot of the Python tests from running (#6407). I'll provide another review in a bit.

@jameslamb jameslamb changed the title [python-package] allow use of early_stopping_round<=0 to turn off early stopping [python-package] allow use of early_stopping_round<=0 to turn off early stopping (fixes #6401) Apr 20, 2024
@jameslamb jameslamb self-requested a review April 20, 2024 02:14
jameslamb
jameslamb approved these changes Apr 20, 2024
View reviewed changes
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much, we'd love to have you come back and contribute again in the future!

@jameslamb jameslamb merged commit 1ce2357 into microsoft:master Apr 20, 2024
42 checks passed
@ddelzell
Copy link
Contributor Author

Thanks, James! Great process, I learned a lot and your reviews were incredibly thorough.

@ddelzell
Copy link
Contributor Author

@jameslamb Do you know when the next release is scheduled to be?

@jameslamb
Copy link
Collaborator

We're hoping to do one within the next month... but we're battling severe issues in CI right now 😭 (#6425).

@ddelzell
Copy link
Contributor Author

OH NO. Thank you for the reply, that's helpful to know.

@jameslamb jameslamb mentioned this pull request Aug 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameslamb jameslamb jameslamb approved these changes

@guolinke guolinke Awaiting requested review from guolinke guolinke is a code owner

@shiyu1994 shiyu1994 Awaiting requested review from shiyu1994 shiyu1994 is a code owner

@jmoralez jmoralez Awaiting requested review from jmoralez jmoralez is a code owner

@borchero borchero Awaiting requested review from borchero borchero is a code owner

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[python-package] Early stopping callback added when early_stopping_round = 0
3 participants
@ddelzell @jameslamb @shiyu1994